[ReviewAgent] Reviewed this seams refactor end-to-end (backend broker/config/telemetry/migrations, CLI container, UI alias rename, arch-facts vendoring). The design is clean, backward-compat is well-reasoned, and the docstrings are excellent. The UI portion is a mechanical @/→@oss-internal/ rename that checks out. A few substantive items before merge:

1. (Correctness / PR-description mismatch) An injected Broker via AppContainer reaches only the sync path, not the async worker.
The PR states: "The async worker honors an injected broker_factory too, so injected brokers reach both the sync and async paths." But AppContainer exposes only broker: Broker | None (an instance), which the factories stash on app.state.broker. The sync router reads app.state.broker ✅. The async worker, however, reads app.state.broker_factory (broker/web/app.py:164) — and nothing bridges container.broker → app.state.broker_factory. AppContainer has no broker_factory field, and no code in shared/web/ ever sets that attribute. So a downstream consumer using the documented container seam gets sync-only injection; the worker silently falls back to default_broker. This is also semantically unavoidable as written: the worker needs a Callable[[UpstreamRunner], Broker] (per-runner), which a single injected instance can't satisfy. Either add a broker_factory field to AppContainer and wire it onto app.state, or correct the PR description / docstrings to state the container's broker is sync-path-only.

2. (Test coverage) The new extension points are largely untested.
The entire value of this PR is the seams, yet the only new seam with direct tests is MigrationTarget/register_target (+ the compliance harness self-test). There are no tests for: AppContainer.extra_routers/extra_installers/broker injection through create_surface_app/create_combined_app; register_config/registered_config_models/AppConfig.extension() (incl. the collision guards); register_telemetry_event/resolve_wire_name (incl. wire-name validation + collision rejection). Given item #1 slipped through precisely because there's no injection test, please add coverage that mounts an app with a populated container and asserts an injected broker is actually invoked on both paths.

3. (Behavior change, not called out) AppConfig now sets extra=\"forbid\".
main had no such constraint. Any existing deployment carrying an unrecognized top-level config key (that isn't a registered extension) will now fail at load_config() instead of silently ignoring it. This is defensible (catches typos/misconfig loudly) and extensions are popped before validation, so the mechanism is sound — but it's a breaking change to config parsing that isn't flagged in the PR body or a migration note. Please confirm no shipped example/deploy configs carry stray keys and note the change.

4. (Non-blocking, worth a comment) Injected sync broker bypasses per-request runner selection.
In execute.py:573-574, when app.state.broker is set, the per-request runner (chosen via RunnerRegistry for the scheme, wrapped with circuit-breaker/bulkhead/deadline) is discarded in favor of the injected instance. The # it owns its own transport comment covers intent, but it means an injected broker opts out of the built-in resilience stack — a non-obvious tradeoff worth stating explicitly in the AppContainer.broker docstring.

5. (Docs) No consumer-facing composition guide.
Docstrings are thorough, but there's no docs/ entry tying the seams together for a downstream integrator (build an AppContainer, register_config, register_target, register_telemetry_event, subclass the jentic_one.testing compliance bases). Per CLAUDE.md's documentation expectation for non-obvious patterns, a short "extending jentic-one" doc would pay off — especially given the CLI/UI/backend seams are meant to be used together.

Nothing here is a security regression — the migration FK-rollback ordering, per-schema version tables, and the SSRF/egress path are untouched or improved. Items #1–#3 are the ones I'd want resolved before squash-merge.

This looks solid and executes perfectly on the 'Dependency Overlay with Strict Seams' directive from the architecture spec (jentic-one-internal#704).

Key takeaways:

• AppContainer in the Go CLI (cli/pkg/core/container.go) successfully models the bi-directional boundary without pulling internal/ outward.
• Moving to DB_TARGETS with register_target instead of a static dict in Alembic resolves the Postgres schema collision danger natively.
• The jentic_one.testing.compliance harnesses are an elegant fix for Python's structural typing gaps (isinstance not validating method signatures).
• The neutral Broker interface decouples the execution layer properly.

A thorough follow-through on the open-core strategy.

[ReviewAgent] Reviewed the seam/extension-point refactor across backend, CLI, UI, and docs. The design is coherent and the bulk of the diff (UI `@/` → `@oss-internal/` alias rename, ~300 files) is mechanical and safe. The config/telemetry/migration registries are clean, well-guarded (collision + idempotency checks), and integrated through single chokepoints (`resolve_wire_name`, `DB_TARGETS`). No secrets introduced; `init-schemas.sql` adds only commented example grants, and `rules-clone.sh` fails soft. Below are asks before merge.

1. (Correctness + docs mismatch — blocking) The injected `AppContainer.broker` never reaches the async worker path, contradicting the PR's headline claim.

The PR description, `docs/development/extending-jentic-one.md`, and the test docstring all state an injected `Broker` is honored by both the sync router and the async worker ("one pipeline, two callers"). But the wiring only closes the sync half:

• `AppContainer` exposes a `broker` instance → the factories set `app.state.broker` (`app_factory.py:383-384`, `432-433`).
• Sync path (`broker/web/routers/execute.py:572-574`) reads `app.state.broker` → ✓ honored.
• Async path (`broker/web/app.py:164-167`) reads `app.state.broker_factory` (a callable) — but nothing anywhere in `src/` ever sets `app.state.broker_factory` (grep confirms only readers, no writers). `AppContainer` has no `broker_factory` field, and neither app factory derives one from `container.broker`.

Net effect: an integrator who follows the docs and injects `AppContainer(broker=MyBroker())` gets it on the sync path, while the async worker silently falls back to `default_broker` (the built-in `DefaultBroker`). That is exactly the "async path that drifted from the sync one" the executor docstring warns against.

`test_async_path_invokes_injected_broker` passes only because it constructs `PipelineExecutor(registry, broker_factory=...)` directly, bypassing `AppContainer` → `app.state` → the broker lifespan. So it proves the plumbing below the gap works but never exercises the container→worker bridge that the docs promise.

Please either (a) have the app factories set `app.state.broker_factory = lambda _runner: container.broker` when `container.broker is not None` (an injected broker owns its transport, so ignoring the runner is correct), or (b) add a `broker_factory` field to `AppContainer` that the factories propagate — and add a test that drives the async path through an `AppContainer`, not around it. Also reconcile the docs claim with whatever is chosen.

2. (Minor inconsistency) `create_surface_app` silently drops the `extra_routers` prefix.

`RouterSpec` is documented as `(router, prefix, tags)`. `create_combined_app` honors it (`root.include_router(router, prefix=prefix, ...)`, `app_factory.py:468-470`), but `create_surface_app` binds it to `_extra_prefix` and drops it (`app_factory.py:385`), mounting the router at root. An integrator passing a prefix to a surface app loses it with no error. Please align the two (apply the prefix in both, or document why surface apps ignore it).

3. (Escalation — `needs-human`) Per `REVIEWS.md` this PR trips two explicit human-review triggers: it changes module boundaries (`shared/`, `broker/`) and modifies `.harness/setup.sh` (agent setup). Adding `needs-human` and deferring final approval to a maintainer. The `.harness` change itself (fetch internal rules, fail-soft, export `JENTIC_RULES_DIR`) looks correct.

Non-blocking observations (no action required): `search_mode` moving `Literal["lexical"]` → `str` defers validation from config-load to first-query in `resolve_strategy` — a documented, acceptable tradeoff. The `extra="forbid"` breaking change is clearly called out in both the PR body and the docs. Widening `TelemetryEvent.name` to `... | str` is consistent with `sink.record` and the `str(event.name)` serialization site.

[ReviewAgent] Reviewed the seam refactor end-to-end (backend DI/config/telemetry/migrations, CLI, UI alias rename, arch-rules vendoring). This is a well-executed, backward-compatible change: the arch boundary holds (shared/ never imports broker/ — verified), value objects were centralized in shared/broker/execution.py with re-exports so existing import sites are untouched, search_mode loosening is safe because resolve_strategy still validates per-dialect at resolve time, SSRF validation stays at the web edge before the broker call (injected brokers can't bypass it), and the 67 new/affected unit+arch tests pass with ruff clean on touched files. The extra="forbid" breaking change is documented in both the PR body and docs/development/extending-jentic-one.md. Nice work on the vendored-facts OSS-safety guard.

One substantive point plus minor notes:

1. The injected-Broker seam can't reach the surface that actually runs a broker. broker/web/app.py::create_app(ctx) takes no container, and __main__._build_app calls mod.create_app(ctx). Because the broker must run as the sole surface (_build_app raises on broker + others), the broker is never part of create_combined_app — which means:

   • the root.state.broker / broker_factory wiring added to create_combined_app is inert (no broker router is mounted there, and its combined worker starts with _start_worker(ctx) and no executor), and
   • the shipped standalone broker binary has no path to inject a Broker via AppContainer without a downstream fully re-implementing broker/web/app.py's assembly (broker_lifespan, admission gate, auth, error handlers, registry resolver).

   The new tests prove create_surface_app/create_combined_app honor a container, and that PipelineExecutor(broker_factory=...) works — but not that a downstream can actually inject a broker into the real broker deployment through the documented AppContainer path. Please either thread container through broker/web/app.py::create_app (and __main__) so the seam is reachable for the sole surface that uses it, or, if the intended model is "downstream calls create_surface_app with the broker's own routers/lifespan directly," document that explicitly in extending-jentic-one.md and drop the inert combined-app broker wiring to avoid the misleading impression it takes effect.

2. Minor — TelemetryEvent.name and TelemetrySink.record were widened to TelemetryEventName | str. Confirmed the only consumer (telemetry/client.py) uses str(event.name), so this is safe; flagging only so future .value-style access on name is avoided.

3. Minor — AppContainer.extra_routers mount in create_surface_app ignores the per-router prefix (uses _prefix throw-away), while create_combined_app applies it. That asymmetry is defensible (surfaces mount at root) but worth a one-line comment so an integrator isn't surprised a prefix is silently dropped in surface mode.

None of these block correctness — default behavior is unchanged and verified. #1 is the one worth resolving before merge since it's the gap between the headline capability and what's wired.

[ReviewAgent] Reviewed the seams work — a well-structured, backward-compatible, and unusually well-documented PR. The 394-file count is dominated by the mechanical UI @/ → @oss-internal/ rename, which I verified is complete and consistent (tsconfig/vite/vitest/eslint aliases all updated; no real @/ import statements remain, only comment prose). The substantive backend/CLI seams are clean: full arch suite passes (256) including module-boundary enforcement, and the new unit tests pass (67). A few asks before merge:

1. Streaming silently bypasses the injected Broker — behavior/doc mismatch (most important). The Broker protocol exposes only execute(), which returns a fully-materialized ExecutionOutcome. But the sync streaming path (broker/web/routers/execute.py::_handle_streaming) calls open_streaming_response(runner, ...) directly against the runner — it never reads app.state.broker/broker_factory. So an injected Broker reaches the buffered-sync and async paths, but not streaming requests. The docs (docs/development/extending-jentic-one.md) and the Broker docstring state the injected broker owns transport/resilience and is honored by "both callers … the sync and async paths, not just one of them" — which reads as full data-plane coverage. A downstream injecting a broker for transport routing, egress policy, or response redaction would have streaming traffic escape it with no warning. This is a security-relevant gap in the seam's stated contract. Please either (a) explicitly document that streaming is not routed through the injected Broker (and why — execute() can't model a lazy body), or (b) extend the seam to cover it. At minimum the docs should not imply total coverage.

2. The sync-path test doesn't actually exercise _handle. test_sync_path_selection_prefers_injected_broker re-implements the injected if injected is not None else broker_factory(runner) selection inline (the test even says "Mirrors broker/web/routers/execute.py"). Because it's a copy of the production expression rather than a call into _handle, it won't catch drift if the real selection logic changes. The async-path test is genuinely end-to-end through run_execution — nice. Consider making the sync assertion drive the real handler (or a thin extracted selector function) so it guards against regressions rather than restating them.

3. search_mode validation moved from load-time to resolve-time — confirm this is intended. Widening Literal["lexical"] → str means an invalid/typo'd search_mode (e.g. "lexcial") now passes config validation and only fails later at resolve_strategy (via SearchUnsupportedError) on first search, rather than loudly at boot. This is slightly at odds with the PR's broader "fail loudly at startup" theme (extra="forbid"). It still fails loudly eventually and the message is good, so this is a judgment call — just flagging the deferred-failure tradeoff in case boot-time rejection was intended.

Non-blocking: the extra="forbid" breaking change is clearly called out in the PR body and docs — good. Nice touch on the vendored-facts OSS-safety guard and the FK-safe reversed downgrade ordering.

[ReviewAgent] Approved — this is a well-executed, genuinely backward-compatible refactor. The seam design is clean and the "no consumer wiring → identical behavior" claim holds up under inspection. Verified locally:

Correctness

• 257 arch tests + 435 broker unit tests + 69 new seam/config/telemetry/migration/compliance tests all pass; mypy clean on the new modules.
• No dangling references after the renames: default_pipeline→default_broker, pipeline=→broker=, and open_streaming_response now routes through DefaultBroker.execute_streaming. All prod + test call sites updated.
• Value-object extraction to shared/broker with re-exports from the old modules keeps every existing import working; module-boundary arch test still green (shared/broker depending on fastapi is arch-neutral by design).
• TelemetryEvent.name widened to TelemetryEventName | str; the one consumer (client.py) uses str(event.name), so plain-string wire names are safe.

Breaking change (extra="forbid") — correctly handled: both shipped configs (config/local.yaml, config/local-sqlite.yaml) load cleanly; registered extension sections are extracted before core validation and round-trip via AppConfig.extension(); unregistered top-level keys are rejected loudly. The change is clearly called out in the PR body and the docs.

Security

• The injected-Broker seam does not bypass the SSRF guard: validate_upstream_url() runs in the router before _resolve_broker() on both the buffered and streaming paths. The only thing an injected broker opts out of is the built-in resilience stack, which is explicitly documented in the Broker protocol docstring and the extending guide.
• register_config / register_telemetry_event / register_target all have collision guards (core-field shadowing, reserved extensions key, wire-name regex, conflicting re-register) with test coverage.
• Rules-facts vendoring: the private clone lands in a gitignored .rules/ path and the loader is read-only, so private rule content can't leak into the public repo; standalone clones self-enforce against the committed vendored subset.

Docs — docs/development/extending-jentic-one.md is a solid, accurate composition guide covering all seams, the wiring order, and the breaking change.

Non-blocking nits (optional):

1. src/jentic_one/shared/jobs/protocols.py:86 — the UpstreamExecutor docstring still reads run_execution(pipeline=default_pipeline(runner)); the kwarg is now broker= (run_execution(broker=default_broker(runner))). Stale reference only.
2. migrations/run.py: with --direction down and no --target, the default -1 is applied per database across the loop, so a bare --db a --db b down-migrates one step in each. That's a reasonable default but worth a one-line note in scripts/migrate.sh help so nobody expects a single global step.